Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

packer: 0.10.1 -> 0.12.1 #21469

Closed
wants to merge 1 commit into from
Closed

packer: 0.10.1 -> 0.12.1 #21469

wants to merge 1 commit into from

Conversation

binarin
Copy link
Contributor

@binarin binarin commented Dec 28, 2016

Update was done using updateScript that was also added by this commit.
Not the prettiest thing, but it works.

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Update was done using new `updateScript`. Not the prettiest thing, but
it works.
@mention-bot
Copy link

@binarin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zimbatm, @cstrahan and @benley to be potential reviewers.

'';

passthru.updateScript = writeScript "update-packer" ''
#!${bash}/bin/bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to do it, but it wasn't able to catch all those extra deps that are specified in vendor/vendor.json in packer repository - resulting .nix-file contained only packer sources itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you try to run make deps in packer source before go2nix save?

Copy link
Member

@kamilchm kamilchm Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, it's something inconsistent in packer vendor dir, it looks like it's half managed by govendor with not all deps checked in into its repo

mv bin/* $out/bin # */
'';

passthru.updateScript = writeScript "update-packer" ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better served with a separate .sh file and substituteInPlace for the PATH and what-not. what do you think, @garbas? Seems not good having this in line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you run shellcheck on this script? It looks pretty complicated, I think shellcheck would be good for maintainability.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably it would be good to have this in a separate file. but i understand why it is done inline, it is just easier to iterate. i would say it is up to a maintainer of the package to decide what is easier for them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joachifm joachifm added the 8.has: package (update) This PR updates a package to a newer version label Jan 1, 2017
@zimbatm
Copy link
Member

zimbatm commented Jan 4, 2017

Hey all. The packer build used to be very complicated but has been made much easier now through vendoring. I went ahead and made another simplified version over here: #21656 , unfortunately it's missing the update script.

@zimbatm
Copy link
Member

zimbatm commented Jan 6, 2017

Is there anything to salvage from this PR?

@binarin
Copy link
Contributor Author

binarin commented Jan 6, 2017

No.

@binarin binarin closed this Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (update) This PR updates a package to a newer version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants